-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MNT-23433] Close Preview button position #3535
Conversation
projects/aca-content/viewer/src/lib/components/viewer/viewer.component.html
Outdated
Show resolved
Hide resolved
dd3902d
to
0e09321
Compare
app/src/app.config.json
Outdated
@@ -1132,6 +1132,7 @@ | |||
"downloadPromptDelay": 50, | |||
"downloadPromptReminderDelay": 30, | |||
"enableFileAutoDownload": true, | |||
"fileAutoDownloadSizeThresholdInMB": 15 | |||
"fileAutoDownloadSizeThresholdInMB": 15, | |||
"isCloseButtonOnLeft": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to make button position configurable I think that this param should accept right
or left
values and also I think the name of the param should be adjusted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is unfixed still, I also mentioned the same improvement in ADF PR
Done
@@ -1177,6 +1177,23 @@ | |||
} | |||
} | |||
] | |||
}, | |||
{ | |||
"id": "app.viewer.separator.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ids of elements should be unique
"order": 11000 | ||
}, | ||
{ | ||
"id": "app.viewer.close", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add new button for that, the close button already exist in ADF and ACA should just set the position value which should be passed to the ADF component which will correctly place the button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichalKinas to move the close button to the right side in the header we have to add the close button in toolbar-actions, and for left position we are using the existing button only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think you have to add new button in ACA? This is another thing that has to be maintained, is not documented properly, has naming issues and rule canClosePreview
doesn't do what it's name suggests. This should be part of ADF component which should take a param with position of close button. This solution works in ACA only and overrides default component behaviour, I still think that the solution should be adding new param for ADF component and component itself should decide how to display the close button
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichalKinas as per your suggestion I have removed the close button from toolbar-actions,now I am planning to add one more button in ADF viewer.component.html which will be show/hide as per the position set by customer in app.config.json which can be configuration from ACA or ADF.
187bff8
to
e0f6e1c
Compare
projects/aca-content/viewer/src/lib/components/viewer/viewer.component.scss
Outdated
Show resolved
Hide resolved
app/src/app.config.json
Outdated
@@ -1132,6 +1132,7 @@ | |||
"downloadPromptDelay": 50, | |||
"downloadPromptReminderDelay": 30, | |||
"enableFileAutoDownload": true, | |||
"fileAutoDownloadSizeThresholdInMB": 15 | |||
"fileAutoDownloadSizeThresholdInMB": 15, | |||
"isCloseButtonOnLeft": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is unfixed still, I also mentioned the same improvement in ADF PR
Done
@@ -13,6 +13,7 @@ | |||
[allowDownload]="false" | |||
[allowFullScreen]="false" | |||
[overlayMode]="true" | |||
[allowGoBack]="'viewer.isCloseButtonOnLeft' | adfAppConfig: false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allowGoBack
should be unchanged here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// todo: remove this when viewer supports extensions | ||
.adf-viewer-toolbar > * > button:last-child { | ||
.adf-viewer-toolbar > * > button:nth-last-child(3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
ee295e9
to
e11c763
Compare
[allowPrint]="false" | ||
[showRightSidebar]="true" | ||
[allowDownload]="false" | ||
[allowFullScreen]="false" | ||
[overlayMode]="true" | ||
[closeButtonPosition]="'viewer.closeButtonPosition' | adfAppConfig: 'right'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check if that works with left
as a default value in ADF? And also if that works if the param is not set in the config file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I checked with default value as left as well working fine and also I try after removing the param from config file, it will come at right by default.
projects/aca-content/viewer/src/lib/components/viewer/viewer.component.html
Outdated
Show resolved
Hide resolved
565d5b3
to
0387629
Compare
94ea35d
to
a6c24e2
Compare
a6c24e2
to
8545baf
Compare
8545baf
to
db141b6
Compare
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
The button to close the preview is located on the left side while the other control buttons are on the right.
What is the new behaviour?
The button to close the preview is now configurable, customer can set the position of close button either left or right.
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information:
https://alfresco.atlassian.net/browse/MNT-23433
Dependent PR: Alfresco/alfresco-ng2-components#9143